Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add soil tillage for crops #2040

Merged
merged 104 commits into from
Jan 10, 2024
Merged

Add soil tillage for crops #2040

merged 104 commits into from
Jan 10, 2024

Conversation

samsrabin
Copy link
Collaborator

@samsrabin samsrabin commented Jun 22, 2023

Description of changes

This PR brings in the tillage code written by Sam Levis and Michael Graham and used in Graham et al. (2021, ERL, doi:10.1088/1748-9326/abe6c6). Low- and high-intensity tillage here work by increasing the decomposition rate of different soil carbon pools. These "decomposition multipliers" vary based on soil pool and how long it's been since the crop was planted; they are set with new paramfile variables till_decompk_multipliers and mimics_till_decompk_multipliers.

NOTE: This PR prevents tillage from actually being enabled. The reasoning is that we want to get the new parameters into the paramfile soon, but scientific testing will take a while.

Specific notes

Contributors other than yourself, if any: Michael Graham (@mwgraham), Sam Levis (@slevis-lmwg), Danica Lombardozzi (@danicalombardozzi)

CTSM Issues Fixed:

Are answers expected to change? No

Any User Interface Changes (namelist or namelist defaults changes)?

  • Adds parameter tillage_mode with options off (default), low, and high.
  • Adds parameter use_original_tillage_phases (false by default; see "Substantive changes" below).
  • Adds parameter max_tillage_depth.

Testing performed, if any:

  • Unit tests pass.
  • aux_clm tests OK (NLFAIL) except for expected failures. Baseline saved as tillage.d43c6178c.

Substantive changes from original @slevis-lmwg / @mwgraham code

  • I left out the dependence of decomposition multipliers on GDP. Code for distinguishing more- vs. less-developed countries was in the original branch, but the "less developed country" option was hard-coded to always be used. (Note that what I brought in reflects the description in Graham et al. (2021).)
  • The original code calculates but ignores "days past planting" (idpp), using instead a calculation that seems incorrect for growing seasons that cross over into a new calendar year. This version uses idpp instead. However, the old version can be used by setting use_original_tillage_phases to true.
  • Default max_tillage_depth is 26 cm. This was the intention of the original code, but it accidentally used 32 cm.

Limitations

  • The effects of tillage on soil physical properties and albedo are not considered.
  • Tillage is either enabled or disabled globally for all CFTs. There is no option to limit it to only some CFTs and/or gridcells.

Questions

Substantive

  • @wwieder: Should I extend this to work with MIMICS decomposition? Doing so would require some scientific thinking and probably literature review. Yes; done.
  • Should I extend this to work with FATES? I think the only reason I disabled that is because I'm unsure how/whether coarse woody debris, which FATES handles, should be affected by tillage. Yes.
  • Should I bring in (an option to enable) GDP dependence, as was in the original code but unused? Unfortunately the "developed country" version uses hard-coded dates to determine decomposition multiplier; we'd surely want to generalize that but it'd require a discussion. Not doing this for now, as it seems to have only been an initial attempt.

Technical

  • Should I move the 54 decomposition multipliers (currently hard-coded) to the parameters file? Yes; done.
  • Is there a simpler way than this to get the active patch associated with a given crop column? Seemingly no.
  • Should I add a test in testlist_clm.xml that uses the till testdef? And/or, should I edit one of the aux_clm tests to use it instead of crop?

Remaining work

  • aux_clm
  • Replace paramfile paths
  • Scientific testing
  • Add Tech Note documentation in Decomposition (link from Crop/irrigation)
  • Add User Guide documentation in "Running special cases"
  • Add till testdef.

samsrabin added 26 commits May 20, 2023 14:58
Each crop column should only have one patch, so by only calling these for crop columns, we can simplify things considerably.
"Throw error during namelist build if tillage is called with FATES"
Update FATES tests to double precision

This pull request updates the fates tests to set the output
precision to double precision.  The usermod fates_sp is similarly
updated.
@samsrabin samsrabin self-assigned this Jun 22, 2023
@samsrabin samsrabin added enhancement new capability or improved behavior of existing capability tag: enh - new science next this should get some attention in the next week or two. Normally each Thursday SE meeting. labels Jun 22, 2023
@samsrabin
Copy link
Collaborator Author

@slevis-lmwg, can I look on your calendar to schedule a re-review?

b4b changes to Python scripts, history lists, tech note, and clm_time_manager.

* Add system and unit tests for making fsurdat with all crops everywhere (ESCOMP#2081)
* Rework master_list* files etc. (ESCOMP#2087)
* Fixes to methane Tech Note (ESCOMP#2091)
* Add is_doy_in_interval() function (ESCOMP#2158)
* Avoid using subprocess.run() in FSURDATMODIFYCTSM (ESCOMP#2125)

Closes issues:
* Add unit test for making fsurdat with all crops everywhere (ESCOMP#2079)
* Rework master_list_(no)?fates.rst? (ESCOMP#2083)
* conda run -n can fail if a conda environment is already active (ESCOMP#2109)
* conda fails to load for SystemTests (ESCOMP#2111)
@slevis-lmwg
Copy link
Contributor

slevis-lmwg commented Oct 3, 2023 via email

@samsrabin
Copy link
Collaborator Author

samsrabin commented Oct 5, 2023

Tasks from re-review with @slevis-lmwg:

  • Delete "Test identicality when using get_prev_days_per_year()" todo c8d6754
  • Correct comment about "uses get_prev_calday() instead of get_curr_calday()". 7510431
  • Do not till generic crops. d565176
  • Warn instead of error when trying to enable tillage, allowing override. b86402e
  • Delete add_tillage_to_paramsfile.py. 1a3a8b7

Additional tasks:

@samsrabin
Copy link
Collaborator Author

I've figured out the reason my runs showed such a small tillage effect relative to Mike's! Turns out, I had incorrectly ordered the dimensions of the *_till_decomp_multipliers variables on the netCDF—they need to be reversed from how they are in the code.

Now that this is fixed (v2), the tillage effect is about as strong as it was with Mike's code.

screenshot_6749

Copy link
Contributor

@slevis-lmwg slevis-lmwg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@samsrabin and I went over updates since I last reviewed this PR. Sam R will change a couple of existing "crop" tests to "till" tests and beyond that this PR is ready.

@samsrabin samsrabin added the PR status: ready PR: this is ready to merge in, with all tests satisfactory and reviews complete label Jan 5, 2024
@samsrabin samsrabin merged commit 3b52933 into ESCOMP:master Jan 10, 2024
2 checks passed
@samsrabin samsrabin added the science Enhancement to or bug impacting science label Aug 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement new capability or improved behavior of existing capability PR status: ready PR: this is ready to merge in, with all tests satisfactory and reviews complete science Enhancement to or bug impacting science
Projects
Status: Ready to eat (Done!)
Archived in project
Development

Successfully merging this pull request may close these issues.

Add "tillage" to bgc-crop model
4 participants